fix: Pass request type to SelectiveDecimalColumnReader#17463
Conversation
✅ Deploy Preview for meta-velox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
meta-cla
Bot
added
the
CLA Signed
Build Impact AnalysisSelective Build Targets (building these covers all 247 affected)Total affected: 247/575 targets Affected targets (247)Directly changed (2)
Transitively affected (245)
Fast path • Graph from main@fd130f44a388ce8ece4ce629fd4c0ceb45b60942 |
jinchengchenghh
left a comment
jinchengchenghh
left a comment
There was a problem hiding this comment.
Thanks for your fix, please add the test
| // in getIntValues(); in that case the Gluten fallback-tag logic will | ||
| // ensure symmetric scan behaviour at the join level. | ||
| : SelectiveColumnReader( | ||
| (requestedType && requestedType->isDecimal() && |
There was a problem hiding this comment.
Why not directly send requestedType to SelectiveColumnReader?
There was a problem hiding this comment.
I'm not sure if other types is OK.
There was a problem hiding this comment.
Please align with
SelectiveIntegerColumnReader(
const TypePtr& requestedType,
dwio::common::FormatParams& params,
velox::common::ScanSpec& scanSpec,
std::shared_ptr<const dwio::common::TypeWithId> fileType)
: SelectiveColumnReader(
requestedType,
std::move(fileType),
params,
scanSpec) {}
The SelectiveColumnReader should decide if it is supported, and throw exception if it is not supported, please also open a PR to verify if this PR can pass all the Gluten unit tests
There was a problem hiding this comment.
The conversion support status is
requestedType_ is the output type the reader materializes, while file storage type comes from fileType_.
- If the file type is narrower than the requested type, the reader widens during materialization via
getFlatValues(), which takes the upcast path inupcastScalarValues(). Examples:int8 -> int16/int32/int64,int32 -> double, and integer/decimal widening in parquet viagetIntValues()plus decimal rescaling inrescaleDecimalValues(). - If the file type is wider than the requested type, it is a narrowing conversion. That is only allowed for explicitly supported numeric paths handled in
getIntValues()orgetUnsignedIntValues(), where the code usescompactScalarValues(). Unsupported combinations fail with checks likeVELOX_FAIL("Unsupported value size")or format-specific errors such as"Result type not supported in ByteRLE encoding". - Semantically, Velox expects schema/type validation to reject unsafe mismatches before decode reaches these paths;
getFlatValues()even documents that invalid cross-domain conversions should have been rejected earlier.
There was a problem hiding this comment.
The SelectiveColumnReader should decide if it is supported, and throw exception if it is not supported, please also open a PR to verify if this PR can pass all the Gluten unit tests
I created a PR apache/gluten#12061 to verify if this PR can pass all the Gluten unit tests. Do we need wait this PR merged, before 12061 could run tests ?
There was a problem hiding this comment.
For std::move(fileType), we get the prompt.
std::move of the const variable 'fileType' has no effect; remove std::move() or make the variable non-const
so we should use fileType directly here.
| // Use requestedType (table schema) when it is a decimal of the same | ||
| // TypeKind as the file type. This allows the reader to rescale values | ||
| // from the ORC file's precision/scale to the metastore-declared | ||
| // precision/scale, so that NativeScan output matches the vanilla Spark |
There was a problem hiding this comment.
Please restrict the comment to table scan, do not include other unrelated information like join
There was a problem hiding this comment.
How about
so that NativeScan output matches the vanilla Spark scan path and decimal join-key comparisons succeed.
->
so that NativeScan output matches the vanilla Spark scan. ?
There was a problem hiding this comment.
// When the read schema (requestedType) differs from the file schema,
// prefer using requestedType for decoding as long as the conversion is
// supported (e.g. reading decimal with different scale). This allows the reader to
// interpret values according to the table schema, so that NativeScan
// matches the vanilla Spark scan behavior.
//
|
And add a brief description about this issue in PR description |
beliefer
force-pushed
the
17462
branch
2 times, most recently
from
bb9f2bc to
86f213b
Compare
| // NativeScan matches the vanilla Spark scan behavior. | ||
| // | ||
| // When the TypeKinds differ (e.g. file is HUGEINT but table declares | ||
| // BIGINT) we fall back to the file type to avoid buffer-size mismatches |
There was a problem hiding this comment.
As the comment on getIntValues, it does not fallback to file type, please remove them
/// Returns integer values for 'rows' cast to the width of 'requestedType' in
/// '*result'.
void getIntValues(
const RowSet& rows,
const TypePtr& requestedType,
VectorPtr* result);
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
Thank you for the fix.
The PR description is still vague — it says "pass the request type into SelectiveColumnReader" but doesn't explain the problem. Please add the explanation of the Hive DECIMAL(38,18) behavior to the PR description.
The code comment says "prefer using requestedType ... as long as the conversion is supported" — but the code unconditionally passes requestedType with no compatibility check or fallback. The comment implies conditional logic that doesn't exist. What happens if requestedType is incompatible (e.g. non-decimal type)? Please add a test for unsupported conversions, and either add the compatibility check or simplify the comment.
The 4 test cases repeat ~30 lines of identical setup (encoding, stream mocks, reader construction). Extract a helper that takes the varying parts (data buffer, scale buffer, file type, requested type, expected values) to eliminate the copy-paste.
Use assertEqualVectors against an expected vector instead of individual EXPECT_EQ calls with C-style (long long) casts.
Use ROW("col_0", DECIMAL(38, 18)) instead of HiveTypeParser().parse("struct<col_0:decimal(38, 18)>").
The background comment in the test (Hive ORC always writes DECIMAL(38,18) in the footer, per-row scale in the SECONDARY stream, reader used footer scale instead of metastore scale) belongs in the API — on the SelectiveDecimalColumnReader constructor or requestedType parameter — not in the test file.
|
@jinchengchenghh Could you explain why you suggested me make the change from to |
|
@mbasmanova Thank you for the comments.
I added the explanation of the Hive DECIMAL(38,18) behavior to the PR description. |
beliefer
force-pushed
the
17462
branch
2 times, most recently
from
87ec8ef to
62ed47f
Compare
CI Failure Analysis
🔴 Expression Fuzzer with Presto SOT — FUZZER Failure View logsAll 4 instances failed (seeds: 1034627224, 858084286, 1012776691, 437388568) 🔴 Join Fuzzer — FUZZER Failure View logs3 of 4 instances failed (seeds: 394900408, 135349143, 522623826) 🔴 Aggregation Fuzzer with Presto as source of truth — FUZZER Failure View logs3 of 4 instances failed (seeds: 580222099, 408420116, 817894527) 🔴 Window Fuzzer with Presto as source of truth — FUZZER Failure View logs1 of 4 instances failed (seed: 568763331) Correlation with PR changes: This PR (#17463) modifies only DWRF decimal column reader files:
The changes pass Known issues:
Reproduce locally: # Expression Fuzzer (any of the 4 seeds will reproduce)
./_build/release/velox/expression/fuzzer/velox_expression_fuzzer_test \
--seed 1034627224 \
--enable_variadic_signatures \
--velox_fuzzer_enable_complex_types \
--lazy_vector_generation_ratio 0.2 \
--common_dictionary_wraps_generation_ratio=0.3 \
--velox_fuzzer_enable_column_reuse \
--velox_fuzzer_enable_expression_reuse \
--enable_dereference \
--duration_sec 300 \
--special_forms="cast,coalesce,if" \
--velox_fuzzer_max_level_of_nesting=1 \
--presto_url=http://127.0.0.1:8080
# Join Fuzzer
./_build/release/velox/exec/fuzzer/velox_join_fuzzer --seed 394900408 --duration_sec 300
# Aggregation Fuzzer
./_build/release/velox/functions/prestosql/fuzzer/velox_aggregation_fuzzer_test --seed 580222099 --duration_sec 300
# Window Fuzzer
./_build/release/velox/functions/prestosql/fuzzer/velox_window_fuzzer_test --seed 568763331 --duration_sec 300Recommended fix: No action needed on this PR. The fuzzer failures are tracked in #17522 and are pre-existing on |
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@beliefer, Thank you for iterating on the feedback!
Test comments are verbose and duplicate the constructor doc. The 5-line block comment before the tests references the constructor comment — redundant. Replace with a one-liner: "Verify that when the file type doesn't match the metastore type, the metastore type wins and data is rescaled accordingly." Per-test comments restate what the test names and parameters already show (e.g., testDecimal128RequestedTypeScaleZero with DECIMAL(38, 18) → DECIMAL(20, 0) is self-explanatory). The // DATA: values 1, 2, 3, 4 as zigzag-encoded varints comment is repeated across tests — the parameter name is sufficient. The 4-line comment on testDecimalRequestedTypeBigintRejected explaining which code path raises the error is implementation detail. Please trim all test comments to only what is non-obvious from the code.
Rejection tests duplicate mock setup. testDecimalRequestedTypeBigintRejected and testDecimalRequestedTypeDoubleRejected have identical mock setup. Please extract a helper or combine into one test that checks both types.
Test names. Drop the test prefix and use shortDecimal / longDecimal instead of Decimal64 / Decimal128 (e.g., testDecimal128RequestedTypeScaleZero → longDecimalRequestedTypeScaleZero).
Simplify call sites.
folly::Range(dataBuffer, std::size(dataBuffer))— just passdataBuffer,folly::Rangeconstructs directly from a C array.{int128_t{1}, int128_t{2}, int128_t{3}, int128_t{4}}— simplify to{1, 2, 3, 4}, the template parameter already specifies the type.ROW({"col_0"}, {DECIMAL(12, 5)})— the helper can wrap the type inROW("col_0", type)internally. Callers should just passDECIMAL(12, 5)andDECIMAL(10, 2).ROW({"col_0"}, {DECIMAL(38, 18)})— drop the braces:ROW("col_0", DECIMAL(38, 18)). Same forROW({"col_0"}, {BIGINT()})etc.
Verify full error message in rejection tests. VELOX_ASSERT_THROW(..., "Schema mismatch") only checks a substring. The actual error includes "From Kind: X, To Kind: Y" — please verify the full message including the specific types to ensure the error is precise.
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
Thank you for the updates! Almost there.
Lint warning. /*read=*/ should be /*readSize=*/ to match the parameter name.
Rejection test loop. Unroll the loop for readability — it's only 2 cases:
VELOX_ASSERT_THROW(
buildReader(ROW("col_0", BIGINT()), fileType, {}, scanSpec.get()),
"Schema mismatch, From Kind: HUGEINT, To Kind: BIGINT");
VELOX_ASSERT_THROW(
buildReader(ROW("col_0", DOUBLE()), fileType, {}, scanSpec.get()),
"Schema mismatch, From Kind: HUGEINT, To Kind: DOUBLE");3 participants
Summary: when the leaf node (Scan operator) in one child of the Join operator is rolled back to Vanilla Spark, while the leaf node (Scan operator) in the other child still uses NativeScan, and the join key used by Join contains the decimal type, Spark ORC reader will prioritize using the decimal type in the table schema (e.g. DECIMAL (20,0)) over the decimal type in the ORC file (e.g. DECIMAL (38,18)) but Native ORC reader only using the decimal type in the ORC file, which ultimately leads to incorrect matching of the join key, resulting in the output of 0 rows after join.
Root cause:
Hive ORC fixed the DECIMAL (38,18) in footer, and the true scale of each row is written in the SECONDRY stream. The original ORC reader used footer's
The use of scale instead of the scale of the meta store (table schema) results in inconsistency between the output of NativeScan and Vanilla Spark.
Fixes: #17462
This PR proposes to pass the request type into
SelectiveColumnReader.This PR could help to fix apache/gluten#11980